Migrate Django middleware to new-style#533
Conversation
New-style middlewares were [introduced][django_1_10_changelog] in Django `1.10`, and also `settings.MIDDLEWARE_CLASSES` was [removed][django_2_0_changelog] in Django 2.0. This change migrates the Django middleware to conform with the new style. This is useful because it will help us solve the pending issue in open-telemetry#391. By having a single entrypoint to the middleware, `__call__`, which is [wrapped with `sync_to_async` just once][call_wrapped] for async requests, we avoid the [issue][asgiref_issue] where a `ContextVar` cannot be reset from a different context. With the current deprecated `MiddlewareMixin` way, both `process_request` and `process_response` were being [wrapped separately with `sync_to_async`][mixin_wrapped], which was the source of the mentioned issue. [django_1_10_changelog]: https://docs.djangoproject.com/en/3.2/releases/1.10/#new-style-middleware [django_2_0_changelog]: https://docs.djangoproject.com/en/3.2/releases/2.0/#features-removed-in-2-0 [call_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/core/handlers/base.py#L54-L57 [mixin_wrapped]: https://github.com/django/django/blob/213850b4b9641bdcb714172999725ec9aa9c9e84/django/utils/deprecation.py#L137-L147 [asgiref_issue]: django/asgiref#267
adamantike
requested review from
a team,
lzchen and
owais
and removed request for
a team
|
I don't think this is a good enough reason to drop support for older versions even if they aren't supported by their authors anymore. This is 9 versions we'll be dropping support for here. The upside doesn't seem to be worth it. I think we have two choices here:
If we really want to use new style middlewares, I'd recommend to go with option 1. today. |
I agree with trying to maintain support for legacy library versions as much as possible. However, there are a few more considerations to bring to the table:
That being said, I've updated the PR to fallback to inheriting from deprecated |
adamantike
force-pushed
the
django-new-style-middleware
branch
from
44da1cb to
c9614bb
Compare
|
@owais, please let me know what do you think about this approach, when you get the time to review it. |
|
Gentle ping, to be able to move forward with #391 |
|
@owais, can you please review this PR again? |
|
Updated to fix changelog's conflict. |
|
@codeboten I think a single package makes more sense. I'll share my thoughts and preferences in the issue you linked. For this PR, I think the use case is too simple to warrant a new package. I think we should not block this PR for #550. |
|
I have resolved merge conflicts again. Is there anything else I can do to move forward with this PR? |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Thanks for getting this PR in 👍!
3 participants
Description
New-style middlewares were introduced in Django
1.10, and alsosettings.MIDDLEWARE_CLASSESwas removed in Django 2.0.This change migrates the Django middleware to conform with the new style, from Django
2.0onwards. This is useful because it will help us solve the pending issue in #391.By having a single entrypoint to the middleware,
__call__, which is wrapped withsync_to_asyncjust once for async requests, we avoid the issue where aContextVarcannot be reset from a different context.With the current deprecated
MiddlewareMixinway, bothprocess_requestandprocess_responsewere being wrapped separately withsync_to_async, which was the source of the mentioned issue.It's worth mentioning that this change maintains compatibility with legacy Django versions.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
tox -e test-instrumentation-djangoDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.